chore(close-button): add structure for s2 migration#6355
Conversation
|
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
TarunAdobe
force-pushed
the
ttomar/migrate-close-button
branch
from
7e643ab to
aa5ff5d
Compare
|
I implemented this close-button scaffold migration with the Cursor agent, then self-reviewed the diff once before marking it ready for review. A few things worth calling out for reviewers: • Cross400 / Cross500 icons — added so l and xl close-button sizes map to the right cross scale (not just Please flag anything that looks off in the scaffold or wiring |
TarunAdobe
force-pushed
the
ttomar/migrate-close-button
branch
2 times, most recently
from
2988c74 to
d84aed0
Compare
| * @internal | ||
| * Not part of the close-button public API. | ||
| */ | ||
| public override pending = false; | ||
|
|
||
| /** | ||
| * @internal | ||
| * Not part of the close-button public API. | ||
| */ | ||
| public override pendingLabel?: string; |
There was a problem hiding this comment.
Does this work? CEM will scan the Buttonbase properties first. Kindly verify this. Keeping it blocking for now!
There was a problem hiding this comment.
works with the plugin that i added. and here's the ticket for broader button base refractor work: https://jira.corp.adobe.com/browse/SWC-2250
There was a problem hiding this comment.
You can abstract the CloseButton required properties in an abstract class in button.base.ts which ButtonBase and CloseButtonBase should extend. This would keep the API surface cleaner even if future components extends ButtonBase.
// button.base.ts
export abstract class InteractiveButtonbase extends SizedMixin(
ObserveSlotText(ObserveSlotPresence(SpectrumElement, '[slot="icon"]'), ''),
{ validSizes: BUTTON_VALID_SIZES }
) {
static override shadowRootOptions: ShadowRootInit = {
...SpectrumElement.shadowRootOptions,
delegatesFocus: true,
};
/**
* Whether the button is disabled. Removes focusability and prevents
* interaction.
*/
@property({ type: Boolean, reflect: true })
public disabled: boolean = false;
/**
* Accessible label forwarded to the internal `<button>` element as
* `aria-label`. Required for icon-only buttons, which have no visible text.
*/
@property({ type: String, attribute: 'accessible-label' })
public accessibleLabel?: string;
protected get hasIcon(): boolean {
return this.slotContentIsPresent;
}
protected get hasLabel(): boolean {
return this.slotHasContent;
}
/**
* Resolves the accessible name for the button from `accessibleLabel` or
* visible text content. Returns `null` when no accessible name is
* determinable.
*
* @internal
*/
protected getResolvedAccessibleName(): string | null {
return this.accessibleLabel ?? (this.textContent?.trim() || null);
}
/**
* Returns the set of attributes that should be forwarded to the internal
* semantic `<button>` element, if not otherwise directly managed.
*
* @internal
*/
protected getForwardedButtonAttributes(): Record<
string,
string | boolean | undefined
> {
return {
disabled: this.disabled,
};
}
protected override update(changedProperties: PropertyValues): void {
super.update(changedProperties);
if (window.__swc?.DEBUG) {
if (this.hasIcon && !this.hasLabel && !this.accessibleLabel) {
window.__swc.warn(
this,
`<${this.localName}> with an icon and no label must have an "accessible-label" attribute to be accessible.`,
'https://opensource.adobe.com/spectrum-web-components/components/button/#icon-only',
{ issues: ['accessible-label'] }
);
}
}
}
export abstract class ButtonBase extends InteractiveButtonbase {
// pending attributes of button stays here
}
// CloseButton.base.ts
export abstract class CloseButtonBase extends InteractiveButtonbase { }
There was a problem hiding this comment.
if we are making a base class for close button it should follow the other base class patterns, i prefer that we do not define multiple base classes in a single file. each one should be their own including the interactive button base
TarunAdobe
force-pushed
the
ttomar/migrate-close-button
branch
from
6670f9e to
58ebd82
Compare
|
CloseButtonBase initially extended ButtonBase, which owns full button semantics including pending / • CEM / Storybook controls (pending showed up on swc-close-button) Close-button is not a pending control (unlike swc-button), so that surface was wrong. First fix: @internal overrides on CloseButtonBase + a custom CEM internal-override-plugin to strip inherited Final fix (latest commit): structural split instead of doc/CEM patching: • SharedButtonBase — sizing, slots, disabled, accessible-label, accessible-name helpers (no pending) Close-button no longer needs @internal pending overrides or the CEM plugin → plugin removed. |
TarunAdobe
force-pushed
the
ttomar/migrate-close-button
branch
from
12446ad to
b0368d9
Compare
There was a problem hiding this comment.
Blocking this! You added a hybrid approach with ButtonBaseCore + SharedButtonbase and PendingMixin, This is creating 2 new classes why not go with pure abstract class hierarchy which will create 1 new class only. Mixin will have type safety and instanceof issues as i have noted.
TarunAdobe
force-pushed
the
ttomar/migrate-close-button
branch
from
b9af1df to
e26d684
Compare
|
note: ci failures are coming from main not from any changes in this branch |
There was a problem hiding this comment.
I would like to advocate that button base be the shared button base. the pending mixin should be applied at the swc button level. if theres disagreement then i will request that shared button base (i would want to discuss the naming of this as well) should be treated like every other base class and have its own directory for discoverability. or we need to discuss architecture of these kinds of components and how we organize them, i can see this coming up with cards too.
There was a problem hiding this comment.
the concepts of base classes is that they store core functionality. i would almost argue that we dont need a core base class per component. i would use one button base for all buttons and then in SWC add the differing functionality that makes them unique components. i thought that was the pattern we were aiming for.
There was a problem hiding this comment.
As per our discussion yesterday I have removed the mixin, created a controller and removed the sharedButtonbase.
So buttonbase is the base that other buttons can use.
One thing though right now that is still there is a closebuttonbase which extends the button base and overrides close button specific size and color properties. Let me know if this is okay or do we want to not hav ea close button base at all.
There was a problem hiding this comment.
@caseyisonit @TarunAdobe I like this pattern except for the fact that it is a bit difficult to scale. In future if 3+ more components uses PendingController you will have 3-4 hosts each copy pasting the same property block. In that case you need to revisit this and extract a PendingButtonBase class or a helper. For now this is a good clean API surface of ButtonBase.
caseyisonit
left a comment
caseyisonit
left a comment
There was a problem hiding this comment.
i would like a discussion or a mini RFC about the shared button base and architecture
Scaffold 2nd-gen close-button in core and swc: CloseButtonBase with static-color and sizing, minimal render and CSS, Storybook stories, and placeholder tests. Wire core package exports and mark Setup complete in the migration status table. Co-authored-by: Cursor <cursoragent@cursor.com>
Wire close-button l/xl to spectrum-css UI icon paths instead of reusing Cross300. Co-authored-by: Cursor <cursoragent@cursor.com>
Rebase onto main with planning docs merged; tick Setup checklist and status table. Co-authored-by: Cursor <cursoragent@cursor.com>
…isible control Add size and icon tokens so Playwright can wait for swc-close-button in overview stories. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Drop pending wiring from the SWC template, mark inherited pending APIs @internal on CloseButton, hide them in Storybook, and document the scope in the migration plan. Co-authored-by: Cursor <cursoragent@cursor.com>
… MDX Restore public variant in CEM for components that redeclare it on the concrete class, while keeping pending hidden when only a descendant marks it @internal (ancestor ButtonBase must not undo CloseButtonBase). Co-authored-by: Cursor <cursoragent@cursor.com>
2nd-gen removes legacy APIs instead of shipping deprecated aliases. Use static-color only on swc-close-button; variant deprecation stays on 1st-gen sp-close-button per migration plan. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove stray blank line after class opening brace so CI prettier check passes. Co-authored-by: Cursor <cursoragent@cursor.com>
…utton Extract shared button semantics into SharedButtonBase and move pending into PendingMixin on ButtonBase only. Close-button extends SharedButtonBase so pending stays off its public API without a CEM override plugin. Co-authored-by: Cursor <cursoragent@cursor.com>
…base Collapse SharedButtonBaseCore and the abstract SharedButtonBase shell into one concrete SharedButtonBase so PendingMixin(SharedButtonBase) shares the same prototype chain as CloseButtonBase extends SharedButtonBase. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the SharedButtonBase/PendingMixin split with a single core ButtonBase and compose pending semantics via PendingController on swc-button so close-button and future pickers share the base without inheriting pending API. Co-authored-by: Cursor <cursoragent@cursor.com>
…ler split Carry main's capture-phase activation guard onto ButtonBase for disabled state and extend swc-button to also suppress pending clicks, including slotted light-DOM icon targets. Co-authored-by: Cursor <cursoragent@cursor.com>
TarunAdobe
force-pushed
the
ttomar/migrate-close-button
branch
from
d838975 to
2981c31
Compare
| } | ||
| } | ||
|
|
||
| protected get handleClick(): (event: Event) => void { |
There was a problem hiding this comment.
This is creating a new function reference on every getter call. You can make this a bound arrow property. very small performance issue but i wanted to let you know.
Avoid allocating a new bound function on every render by replacing the handleClick getter with a readonly arrow property. Co-authored-by: Cursor <cursoragent@cursor.com>
4 participants
Description
Phase 2 (Setup) of the
swc-close-button1st-gen → 2nd-gen washing machine migration. This PR scaffolds the component so it builds, registers, and appears in Storybook before API polish and S2 styling land in follow-up PRs.Core (
2nd-gen/packages/core/components/close-button/)CloseButton.types.ts—CLOSE_BUTTON_VALID_SIZES, static-color / variant typesCloseButton.base.ts— extends ButtonBase with close-button VALID_SIZES, static-color, and legacy variant(maps to static-color when set; full deprecation warnings deferred to Phase 3). Size uses shared SizedMixin
behavior from ButtonBase (default m, reflected on the host).
core/package.jsonSWC (
2nd-gen/packages/swc/components/close-button/)CloseButton.ts— inner<button type="button">, size-mapped cross icon, visually hidden default slot,aria-labelfrom resolved accessible nameclose-button.css— stub layout and visually hidden label styles (not S2 visual parity)swc-close-button.ts— element registrationDocs
Motivation and context
Close button migration is tracked under the 2nd-gen component workstream. Setup must land before Phase 3 (API / TypeScript) and Phase 5 (S2 CSS). This PR intentionally ships a minimal, buildable skeleton — not visual parity with Spectrum 2 closebutton CSS.
Related issue(s)
(#6322)
Screenshots (if appropriate)
N/A — stub styling only; no meaningful visual regression vs Spectrum 2
yet.
Author's checklist
m-web-components/blob/main/CONTRIBUTING.md) and
[PULL_REQUESTS](https://github.com/adobe/spectrum-web-components/blob
/main/PULL_REQUESTS.md) documents.
see: Aria Practices
Storybook + a11y snapshot; full coverage in Phase 6).
published.
(status table only).
Reviewer's checklist
number without a link
include
patch,minor, ormajorfeatureswriting
Manual review test cases
yarn buildfrom repo root (or2nd-genworkspace build forcore+swc).branch preview Storybook if available).
<swc-close-button accessible-label="Close">renders witha cross icon.
<button type="button">witharia-label="Close".sizetos,m,l, andxl; confirm thehost updates and the icon mapping changes
static-colortowhiteorblack; confirm classesswc-CloseButton--staticWhite/swc-CloseButton--staticBlackappearon the inner button (styling is stub-only).
Device review
Accessibility testing checklist
a visible focus indicator on the inner button (delegated focus).
activates (click handler / default button behavior).
disabledon the control; confirm the button is not reachablevia Tab and does not activate.
Close (from
accessible-labelor slotted text).disabledset, confirm the control is announced asunavailable/disabled.